Use async/await instead of manual polling of `NetworkWorker` by dmitry-markin · Pull Request #13219 · paritytech/substrate

dmitry-markin · 2023-01-23T15:12:54Z

Resolves #6919. Re-does #5704.

I also tried rewriting sc-network-test to use async/await instead of polling, the first attempt is here: 4b5d851. It turned out that doing it without slowing down the tests is less trivial than I expected, so I think we should do it in a separate PR.

cumulus companion: paritytech/cumulus#2127

…work-test`" This reverts commit 4b5d851.

…async-await

melekes

👍

client/network/src/service.rs

koute · 2023-02-06T06:22:15Z

It'd be interesting to see the impact of this on the CPU usage; the original comment here said that there's a high volume of messages here, so this might end up being less efficient overall. If so we can probably do some further refactoring to make it better.

client/network/src/service.rs

bkchr

I don't get why we need next_action. We should just rename it to async fc run. When this run function returns, it means that the worker is finished. I don't think we need any next_action?

client/network/src/service.rs

altonen · 2023-02-06T12:18:49Z

I don't get why we need next_action. We should just rename it to async fc run. When this run function returns, it means that the worker is finished. I don't think we need any next_action?

next_action() is probably useful in tests where we want to advance the network by one event and then observe effects. There could be another function async fn run() which would just call next_event() in a loop.

dmitry-markin · 2023-02-06T12:40:47Z

I don't get why we need next_action. We should just rename it to async fc run. When this run function returns, it means that the worker is finished. I don't think we need any next_action?

@bkchr: should be resolved now.

dmitry-markin · 2023-02-06T13:57:33Z

It'd be interesting to see the impact of this on the CPU usage; the original comment here said that there's a high volume of messages here, so this might end up being less efficient overall. If so we can probably do some further refactoring to make it better.

@koute What would be the proper profiling procedure in this case? Is running the node on a VPS and just measuring the CPU load enough (after sync?), or something more tricky is needed? How to account for varying conditions across the runs (peer count, transactions per block, etc.)?

…nstead of `NetworkService`

…async-await

dmitry-markin · 2023-02-15T13:37:49Z

The PR is ready for review again. @bkchr @altonen could you review the changes since faec2e9?

bkchr

Some things still need to be done, but looks good!

client/network/test/src/lib.rs

client/network/src/service/tests/mod.rs

client/network/src/service/tests/chain_sync.rs

bkchr · 2023-02-17T13:38:36Z

client/network/src/service.rs

+			let external_addresses =
+				self.network_service.external_addresses().map(|r| &r.addr).cloned().collect();
+			*self.external_addresses.lock() = external_addresses;
+
+			let listen_addresses =
+				self.network_service.listeners().map(ToOwned::to_owned).collect();
+			*self.listen_addresses.lock() = listen_addresses;


I still don't think that the listen addresses are updated ever. You pass them via cli, how should the listen address change? Or are these the addresses other nodes have reported to us?

Nevertheless, there we should create an issue to get rid off these mutexes.

It turned out that they do change during runtime — if we listen on a wildcard addresses like 0.0.0.0 and ::, Swarm reports all actual endpoints we are listening on (enumerating all interfaces), and if we enable an interface / assign an ip address (like creating a dummy interface) it pops up in the list of addresses reported by RPC.

dmitry-markin · 2023-02-20T12:07:53Z

bot merge

…ech#13219) * Convert `NetworkWorker::poll()` into async `next_action()` * Use `NetworkWorker::next_action` instead of `poll` in `sc-network-test` * Revert "Use `NetworkWorker::next_action` instead of `poll` in `sc-network-test`" This reverts commit 4b5d851. * Fix `sc-network-test` to poll `NetworkWorker::next_action` * Fix `sc_network::service` tests to poll `NetworkWorker::next_action` * Fix docs * kick CI * Factor out `next_worker_message()` & `next_swarm_event()` * Error handling: replace `futures::pending!()` with `expect()` * Simplify stream polling in `select!` * Replace `NetworkWorker::next_action()` with `run()` * Apply suggestions from code review Co-authored-by: Bastian Köcher <git@kchr.de> * minor: comment * Apply suggestions from code review Co-authored-by: Bastian Köcher <git@kchr.de> * Print debug log when network future is shut down * Evaluate `NetworkWorker::run()` future once before the loop * Fix client code to match new `NetworkService` interfaces * Make clippy happy * Apply suggestions from code review Co-authored-by: Bastian Köcher <git@kchr.de> * Apply suggestions from code review Co-authored-by: Bastian Köcher <git@kchr.de> * Revert "Apply suggestions from code review" This reverts commit 9fa646d. * Make `NetworkWorker::run()` consume `self` * Terminate system RPC future if RPC rx stream has terminated. * Rewrite with let-else * Fix comments * Get `best_seen_block` and call `on_block_finalized` via `ChainSync` instead of `NetworkService` * rustfmt * make clippy happy * Tests: schedule wake if `next_action()` returned true * minor: comment * minor: fix `NetworkWorker` rustdoc * minor: amend the rustdoc * Fix bug that caused `on_demand_beefy_justification_sync` test to hang * rustfmt * Apply review suggestions --------- Co-authored-by: Bastian Köcher <git@kchr.de>

* Convert `NetworkWorker::poll()` into async `next_action()` * Use `NetworkWorker::next_action` instead of `poll` in `sc-network-test` * Revert "Use `NetworkWorker::next_action` instead of `poll` in `sc-network-test`" This reverts commit 4b5d851. * Fix `sc-network-test` to poll `NetworkWorker::next_action` * Fix `sc_network::service` tests to poll `NetworkWorker::next_action` * Fix docs * kick CI * Factor out `next_worker_message()` & `next_swarm_event()` * Error handling: replace `futures::pending!()` with `expect()` * Simplify stream polling in `select!` * Replace `NetworkWorker::next_action()` with `run()` * Apply suggestions from code review Co-authored-by: Bastian Köcher <git@kchr.de> * minor: comment * Apply suggestions from code review Co-authored-by: Bastian Köcher <git@kchr.de> * Print debug log when network future is shut down * Evaluate `NetworkWorker::run()` future once before the loop * Fix client code to match new `NetworkService` interfaces * Make clippy happy * Apply suggestions from code review Co-authored-by: Bastian Köcher <git@kchr.de> * Apply suggestions from code review Co-authored-by: Bastian Köcher <git@kchr.de> * Revert "Apply suggestions from code review" This reverts commit 9fa646d. * Make `NetworkWorker::run()` consume `self` * Terminate system RPC future if RPC rx stream has terminated. * Rewrite with let-else * Fix comments * Get `best_seen_block` and call `on_block_finalized` via `ChainSync` instead of `NetworkService` * rustfmt * make clippy happy * Tests: schedule wake if `next_action()` returned true * minor: comment * minor: fix `NetworkWorker` rustdoc * minor: amend the rustdoc * Fix bug that caused `on_demand_beefy_justification_sync` test to hang * rustfmt * Apply review suggestions --------- Co-authored-by: Bastian Köcher <git@kchr.de>

…ech#13219) * Convert `NetworkWorker::poll()` into async `next_action()` * Use `NetworkWorker::next_action` instead of `poll` in `sc-network-test` * Revert "Use `NetworkWorker::next_action` instead of `poll` in `sc-network-test`" This reverts commit 4b5d851. * Fix `sc-network-test` to poll `NetworkWorker::next_action` * Fix `sc_network::service` tests to poll `NetworkWorker::next_action` * Fix docs * kick CI * Factor out `next_worker_message()` & `next_swarm_event()` * Error handling: replace `futures::pending!()` with `expect()` * Simplify stream polling in `select!` * Replace `NetworkWorker::next_action()` with `run()` * Apply suggestions from code review Co-authored-by: Bastian Köcher <git@kchr.de> * minor: comment * Apply suggestions from code review Co-authored-by: Bastian Köcher <git@kchr.de> * Print debug log when network future is shut down * Evaluate `NetworkWorker::run()` future once before the loop * Fix client code to match new `NetworkService` interfaces * Make clippy happy * Apply suggestions from code review Co-authored-by: Bastian Köcher <git@kchr.de> * Apply suggestions from code review Co-authored-by: Bastian Köcher <git@kchr.de> * Revert "Apply suggestions from code review" This reverts commit 9fa646d. * Make `NetworkWorker::run()` consume `self` * Terminate system RPC future if RPC rx stream has terminated. * Rewrite with let-else * Fix comments * Get `best_seen_block` and call `on_block_finalized` via `ChainSync` instead of `NetworkService` * rustfmt * make clippy happy * Tests: schedule wake if `next_action()` returned true * minor: comment * minor: fix `NetworkWorker` rustdoc * minor: amend the rustdoc * Fix bug that caused `on_demand_beefy_justification_sync` test to hang * rustfmt * Apply review suggestions --------- Co-authored-by: Bastian Köcher <git@kchr.de>

dmitry-markin added 5 commits January 18, 2023 17:48

Convert NetworkWorker::poll() into async next_action()

fb272a6

Use NetworkWorker::next_action instead of poll in sc-network-test

4b5d851

Revert "Use NetworkWorker::next_action instead of poll in `sc-net…

1c87141

…work-test`" This reverts commit 4b5d851.

Fix sc-network-test to poll NetworkWorker::next_action

dbd8ad9

Fix sc_network::service tests to poll NetworkWorker::next_action

e8d0758

dmitry-markin requested review from altonen, bkchr and melekes January 23, 2023 15:12

Fix docs

3071b05

dmitry-markin mentioned this pull request Jan 23, 2023

Use async/await instead of manual polling of NetworkWorker paritytech/cumulus#2127

Closed

dmitry-markin added 2 commits January 23, 2023 19:00

Merge remote-tracking branch 'origin/master' into dm-network-service-…

431e550

…async-await

kick CI

d7be660

melekes approved these changes Jan 24, 2023

View reviewed changes

koute reviewed Feb 1, 2023

View reviewed changes

client/network/src/service.rs Outdated Show resolved Hide resolved

Factor out next_worker_message() & next_swarm_event()

78cce1a

koute approved these changes Feb 6, 2023

View reviewed changes

altonen approved these changes Feb 6, 2023

View reviewed changes

client/network/src/service.rs Outdated Show resolved Hide resolved

client/network/src/service.rs Outdated Show resolved Hide resolved

bkchr suggested changes Feb 6, 2023

View reviewed changes

client/network/src/service.rs Outdated Show resolved Hide resolved

client/network/src/service.rs Outdated Show resolved Hide resolved

dmitry-markin added 2 commits February 6, 2023 14:42

Error handling: replace futures::pending!() with expect()

c9f75f7

Simplify stream polling in select!

4a85d1f

Replace NetworkWorker::next_action() with run()

c165185

dmitry-markin requested a review from bkchr February 6, 2023 12:43

dmitry-markin added 3 commits February 13, 2023 20:55

Get best_seen_block and call on_block_finalized via ChainSync i…

e669c6a

…nstead of `NetworkService`

rustfmt

6ed5d48

make clippy happy

ab1ced4

bkchr mentioned this pull request Feb 14, 2023

add warp to target block for parachains #12761

Merged

dmitry-markin added 8 commits February 14, 2023 12:07

Tests: schedule wake if next_action() returned true

4be62c7

minor: comment

5e90af0

Merge remote-tracking branch 'origin/master' into dm-network-service-…

a9123a7

…async-await

minor: fix NetworkWorker rustdoc

113a888

minor: amend the rustdoc

ac1452c

Fix bug that caused on_demand_beefy_justification_sync test to hang

5fc3831

Merge remote-tracking branch 'origin/master' into dm-network-service-…

6005109

…async-await

rustfmt

9df7d0e

dmitry-markin requested review from altonen and bkchr February 15, 2023 13:37

bkchr approved these changes Feb 17, 2023

View reviewed changes

bkchr reviewed Feb 17, 2023

View reviewed changes

Apply review suggestions

812c316

altonen approved these changes Feb 20, 2023

View reviewed changes

paritytech-processbot bot merged commit 76208c8 into master Feb 20, 2023

paritytech-processbot bot deleted the dm-network-service-async-await branch February 20, 2023 12:08

kiltbot mentioned this pull request Apr 3, 2023

[AUTOMATIC] Update Polkadot dependencies from 0.9.38 to 0.9.39 KILTprotocol/kilt-node#495

Closed

kacperzuk-neti mentioned this pull request Jun 23, 2023

Polkadot v0.9.43 liberland/liberland_substrate#295

Merged

15 tasks

MOZGIII mentioned this pull request Nov 8, 2023

Tracking issue to bump substrate related deps to polkadot-v0.9.39 humanode-network/humanode#828

Closed

11 tasks

Conversation

dmitry-markin commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

koute commented Feb 6, 2023

Uh oh!

Uh oh!

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

altonen commented Feb 6, 2023

Uh oh!

dmitry-markin commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitry-markin commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitry-markin commented Feb 15, 2023

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkchr Feb 17, 2023

Choose a reason for hiding this comment

Uh oh!

dmitry-markin Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitry-markin commented Feb 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dmitry-markin commented Jan 23, 2023 •

edited

Loading

dmitry-markin commented Feb 6, 2023 •

edited

Loading

dmitry-markin commented Feb 6, 2023 •

edited

Loading

dmitry-markin Feb 20, 2023 •

edited

Loading